AP_HAL: clean up UART driver storage/access and legacy ordering #25589

tpwrules · 2023-11-21T02:33:59Z

Eliminates UB and saves 108 bytes on CubeOrangePlus. See commits for details.

No functional change. I did test this on a real board and confirm using a GPS that the port ordering is the same.

Remaining shadows of the legacy ordering:

TCP port numbers on SITL (changing will break users)
ESP32 UART indices (can't test, don't want to fiddle with)
SITL and Linux command line arguments (removing will break users, deprecated with warning)

peterbarker

LGTM.

It's progress (removing uartX as members is good), but there's obviously still more to do here.

Next step would probably be to push that mapping back down into each of the HALs, so the HALs can push the devices into here in a 1-1 order.

libraries/AP_HAL/HAL.h

tridge

I want to get rid of the old mapping completely, just needs some work in HAL_SITL, HAL_Linux etc

tpwrules · 2023-11-21T17:12:28Z

I want to get rid of the old mapping completely, just needs some work in HAL_SITL, HAL_Linux etc

I looked into this but the old mapping still is user-visible as command line options with those. Is it finally time for those options to be removed? That could break people's scripts.

rmackay9 · 2023-11-22T07:12:45Z

@tridge how do you feel about merging this now and then do the AP_HAL changes in a follow-up PR? Just thinking of scope creep and helping devs cut their teeth on this smaller change.

tpwrules · 2023-12-11T20:23:14Z

I spent some cathartic time with Ctrl+F and I think I've managed to eliminate virtually every trace of the old mapping. A couple things remain which might break users if removed. It shouldn't be much effort to remove them for good if desired.

tridge · 2023-12-11T20:52:34Z

@tpwrules wow, this is great! I've been putting this off for ages, and you did it exactly the way I wanted it done. I think you deserve for medal for taking this on.
We just need some testing of this on ChibiOS and SITL at least before merging. I can't spot any errors, but it is such a tricky conversion I think we need to do some careful testing

tpwrules · 2023-12-11T22:44:24Z

Thank you very much for the kind words. Glad to be able to help.

I did test one serial port with a serial GPS on my own CubeOrange to make sure that ordering did not change. SITL should be covered by the CI. I did also launch and fly around a bit a SITL system on my computer using MAVProxy and sim_vehicle.py but was not particularly exhaustive.

tpwrules · 2023-12-12T17:44:44Z

Warnings have been added as discussed on the devcall. Should be ready to merge once you are happy with ChibiOS testing.

magicrub · 2023-12-12T22:59:57Z

Tools/autotest/sim_vehicle.py

@@ -669,7 +669,6 @@ def start_antenna_tracker(opts):
 def start_CAN_Periph(opts, frame_info):
    """Compile and run the sitl_periph"""

-    global can_uarta


Missed one?

That's the only place that name is referenced in the whole source code. I figured it was unused, so I just removed it.

Avoids UB-inducing assumption that UART drivers are consecutive in the serial() function.

Saves a bit of flash and execution time.

Saves a bit more flash.

SERIAL_ORDER has been around for a few years now and UART_ORDER is rejected by the hwdef script, so support for UART_ORDER and associated processing in the hwdef script is removed, along with the order conversion script.

Fourth serial port (SERIAL2) added purely for consistency.

Leave the legacy command line arguments to avoid breaking users.

Legacy command line arguments are kept to avoid breaking users. The vestigial `_tcp_client_addr` variable is removed. Serial port status messages are updated to a slightly different format to clarify the numbering scheme being used and prompt any external consumers to update.

Also delete some unused variables and update the completions.

tpwrules · 2023-12-13T16:14:34Z

Final rebase, mostly to clean up some super minor things and make CI green.

tridge · 2023-12-18T19:52:32Z

@tpwrules I tested all uarts on cubeorange with no issues

peterbarker approved these changes Nov 21, 2023

View reviewed changes

libraries/AP_HAL/HAL.h Outdated Show resolved Hide resolved

peterbarker added the DevCallEU label Nov 21, 2023

tridge requested changes Nov 21, 2023

View reviewed changes

tpwrules force-pushed the ap-hal-uart-cleanup branch 2 times, most recently from 4f3ef57 to cac5009 Compare November 21, 2023 16:46

rmackay9 added DevEnv and removed DevCallEU labels Nov 22, 2023

andyp1per added DevCallTopic and removed DevEnv labels Nov 22, 2023

CraigElder removed the DevCallTopic label Nov 27, 2023

tpwrules force-pushed the ap-hal-uart-cleanup branch 3 times, most recently from 8948024 to c82077e Compare December 11, 2023 20:20

tpwrules changed the title ~~AP_HAL: clean up UART driver storage/access~~ AP_HAL: clean up UART driver storage/access and legacy ordering Dec 11, 2023

tridge added DevCallTopic NeedsTesting labels Dec 11, 2023

CraigElder removed the DevCallTopic label Dec 11, 2023

tpwrules mentioned this pull request Dec 12, 2023

Excise references to legacy and deprecated UART ordering ArduPilot/ardupilot_wiki#5653

Merged

magicrub reviewed Dec 12, 2023

View reviewed changes

tpwrules added 5 commits December 13, 2023 09:56

AP_HAL: use array of UART drivers instead of consecutive variables

3835b17

Avoids UB-inducing assumption that UART drivers are consecutive in the serial() function.

AP_HAL: move serial re-mapping to driver array initialization

fe816cb

Saves a bit of flash and execution time.

AP_HAL: inline serial accessor function

dbb32ff

Saves a bit more flash.

AP_HAL: constructor now accepts UARTs in SERIALn order

c0f2e03

AP_HAL: remove references to legacy UART order

651bca6

tpwrules added 22 commits December 13, 2023 10:02

AP_HAL_Empty: pass UARTs to AP_HAL in SERIALn order

a4571db

AP_HAL_ESP32: pass UARTs to AP_HAL in SERIALn order

32155f5

AP_HAL_Linux: pass UARTs to AP_HAL in SERIALn order

b62d197

AP_HAL_SITL: pass UARTs to AP_HAL in SERIALn order

82bc8fc

AP_HAL_ChibiOS: eliminate legacy UART ordering/references

31f2cca

SERIAL_ORDER has been around for a few years now and UART_ORDER is rejected by the hwdef script, so support for UART_ORDER and associated processing in the hwdef script is removed, along with the order conversion script.

AP_HAL_ChibiOS: remove references to legacy UART order from hwdefs

00ead24

AP_HAL_Empty: eliminate legacy UART ordering/references

8410432

Fourth serial port (SERIAL2) added purely for consistency.

AP_HAL_ESP32: eliminate legacy UART ordering/references

3a0f527

AP_HAL_Linux: eliminate internal legacy UART ordering/references

e70abad

Leave the legacy command line arguments to avoid breaking users.

SITL: remove references to legacy UART order incl. sim arguments

c136da1

Tools: remove references to legacy UART order

13eecc2

Also delete some unused variables and update the completions.

Tracker: remove references to legacy UART order

4c42442

Plane: remove references to legacy UART order

087219d

Rover: remove references to legacy UART order

2d7dea9

GCS_MAVLink: correct comment about stream array

87a98e3

AP_Scripting: remove references to legacy UART order

6d48866

AP_DDS: remove references to legacy UART order

aac2222

AP_FETtecOneWire: remove references to legacy UART order

080140f

AP_SerialManager: clarify comment regarding legacy UART order

bb5193c

AP_HAL_SITL: deprecate and warn on legacy --uartX option use

d78aa8b

AP_HAL_Linux: deprecate and warn on legacy --uartX option use

037e00c

tpwrules force-pushed the ap-hal-uart-cleanup branch from d1807a1 to 037e00c Compare December 13, 2023 16:14

tpwrules requested a review from tridge December 13, 2023 17:27

tridge approved these changes Dec 18, 2023

View reviewed changes

tridge merged commit e460a19 into ArduPilot:master Dec 18, 2023
88 checks passed

tpwrules deleted the ap-hal-uart-cleanup branch December 18, 2023 22:08

tpwrules mentioned this pull request Nov 24, 2024

AP_HAL_ESP32: UART driver tweaks and making closer in shape to chibios #28717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_HAL: clean up UART driver storage/access and legacy ordering #25589

AP_HAL: clean up UART driver storage/access and legacy ordering #25589

tpwrules commented Nov 21, 2023 •

edited

Loading

peterbarker left a comment

tridge left a comment

tpwrules commented Nov 21, 2023

rmackay9 commented Nov 22, 2023

tpwrules commented Dec 11, 2023 •

edited

Loading

tridge commented Dec 11, 2023

tpwrules commented Dec 11, 2023

tpwrules commented Dec 12, 2023 •

edited

Loading

magicrub Dec 12, 2023

tpwrules Dec 13, 2023

tpwrules commented Dec 13, 2023

tridge commented Dec 18, 2023

AP_HAL: clean up UART driver storage/access and legacy ordering #25589

AP_HAL: clean up UART driver storage/access and legacy ordering #25589

Conversation

tpwrules commented Nov 21, 2023 • edited Loading

peterbarker left a comment

Choose a reason for hiding this comment

tridge left a comment

Choose a reason for hiding this comment

tpwrules commented Nov 21, 2023

rmackay9 commented Nov 22, 2023

tpwrules commented Dec 11, 2023 • edited Loading

tridge commented Dec 11, 2023

tpwrules commented Dec 11, 2023

tpwrules commented Dec 12, 2023 • edited Loading

magicrub Dec 12, 2023

Choose a reason for hiding this comment

tpwrules Dec 13, 2023

Choose a reason for hiding this comment

tpwrules commented Dec 13, 2023

tridge commented Dec 18, 2023

tpwrules commented Nov 21, 2023 •

edited

Loading

tpwrules commented Dec 11, 2023 •

edited

Loading

tpwrules commented Dec 12, 2023 •

edited

Loading